Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update secret label when getting with both id and label #172

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

tonyandrewmeyer
Copy link
Collaborator

@tonyandrewmeyer tonyandrewmeyer commented Aug 12, 2024

When we get a secret and provide both ID and label, the label should update to the provided one. This was previously missing in Scenario.

Fixes #95

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Collaborator

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@@ -424,6 +427,9 @@ def secret_info_get(
label: Optional[str] = None,
) -> SecretInfo:
secret = self._get_secret(id, label)
# If both the id and label are provided, then update the label.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be doing this in _get_secret?
we-re side-effecting anyway, so...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered about that. _get_secret is used by three other methods - for grant/revoke it's not relevant, but the code would skipped anyway because the label will always be None. For set, it is relevant, but it's already done because it's updating potentially all the metadata.

So it's either updated twice in set or there's the code duplication. Maybe it's just cleaner in _get_secret so that's the better choice.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm right. I'm 50-50 on this one, I don't mind either way ;)

@tonyandrewmeyer tonyandrewmeyer merged commit 9b85ed6 into canonical:7.0 Aug 12, 2024
2 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the secret-label-fix-95 branch August 12, 2024 22:20
tonyandrewmeyer added a commit that referenced this pull request Sep 2, 2024
When we get a secret and provide both ID and label, the label should
update to the provided one. This was previously missing in Scenario.

Fixes #95
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants